These patches were submitted by Duncan Coutts by email. I've turned them into a pull request for tracking purposes.

About the first two:

The first is to fix the support for HTTP digest authentication. The code
seems to do everything right except that it misformats the md5 digests.
(This is slightly odd because it cannot ever have worked like this.
Presumably someone tested it previously and then the md5 formatting got
broken later.)

Credit for this one goes to Matthew Gruen. We've tested this new code
against happstack, but not against e.g. apache (though we do know the
happstack server-side digest code works with various standard web
browsers).

We will be relying on this first fix in future so that cabal-install
(and all other Haskell clients like the mirroring client) can
authenticate with the hackage-server.

The second fix is for the connection pool logic in the Browser
module/monad.

The browser monad keeps its pool of open connections so that it can
reuse connections rather than creating new ones. Obviously it is
important that when it tries to reuse a connection, that it picks a
correct one.

Currently the connections in the pool are only identified by the
hostname in the URL, not the combinator of hostname and port number.
This means that if you have open connections for say, localhost and
localhost:8080 then when connections are reused it'll pick one of these
arbitrarily which can obviously lead to talking to the wrong server and
failure.

Internally the HandleStream records the host name and isTCPConnectedTo
looks at that.

[ Generally the connection pool logic is a bit odd, instead of filtering
by isTCPConnectedTo, I'd have thought it more sensible to keep a finite
map from (host, port) to connection. isTCPConnectedTo also checks (in an
odd way) that the connection is still open. If it turns out it has been
closed, the connection is not removed from the pool, rather a new
connection is opened instead and the old connections eventually makes
its way to the back of the connection list, which means in the mean time
we could well end up expiring and closing perfectly good connections. I
realise of course you didn't write this mess ]

I've fixed it by instead of relying on the hostname recorded inside the
HandleStream via isTCPConnectedTo, keeping the hostname and port number
in the connection pool itself in the Browser. So then when picking a
connection we filter by the hostname and port number and finally check
if the connection is still connected.

• conn <- ioAction $ filterM (\c -> c isTCPConnectedTo uriAuthToString hst) pool
• conn <- ioAction $ filterM isTCPConnected

•                         [ c | (hostname,portnum, c) <- pool
  

•                             , hostname == uriRegName hst
  

•                             , portnum  == uriAuthPort Nothing hst ]
  

To do that I've introduced variants of isTCPConnectedTo and
isConnectedTo that just test if they're connected and don't take any
hostname as a parameter. As far as I can see both isTCPConnectedTo and
isConnectedTo are pretty pointless functions so I've also taken the
liberty of deprecating them (the module they're in is mostly internal
anyway).

I've tested this fix with my hackage-mirror client, mirroring between
localhost and localhost:8080.

And about the third:

Oh, and another one!

This is a fix to Network/Browser.hs to return 304 "not modified"
responses directly and not to treat them as a redirection (since it's
not a redirection).

The 304 code is returned by servers in response to conditional GET/PUT
requests (e.g. GET met this resource, but only if it changed more
recently than time t), to say that the resource has not changed. This
conditional stuff is needed to validate client-side caches. I'm now
using this in the hackage-mirror and it would be nice to have it in
cabal-install too, as it'd make cabal update a bit cheaper.

You may notice that this partially reverts a patch from aslatter from
last year. He changed:

•  (3,0,x) | x `elem` [2,3,1,7]  ->  do
  

•  (3,0,x) | x /= 5  ->  do
  

and I've changed it back

•  (3,0,x) | x /= 5  ->  do
  

•  (3,0,x) | x `elem` [2,3,1,7]  ->  do
  

This isn't really the important part of the patch, the important bit is:

•  (3,_,_) -> redirect uri rsp
  

  _ -> return (Right (uri,rsp))

http://en.wikipedia.org/wiki/List_of_HTTP_status_codes#3xx_Redirection

What is going on is that there are four "30x" response codes that should
be treated as redirect: 301, 302, 303 and 307.

There are a few other "30x" codes which are not redirects, or not
standard redirects: 300, 304, 305 and 306. The current code handles 305
explicitly. The current code treats 300 as a redirect too, using the
catch-all case:

  (3,_,_) -> redirect uri rsp

While the HTTP spec says that clients MAY treat 300 as a redirect,
that's only if the server provides a location, which is optional for
300. The current redirect code always expects a location, so it's been
handling 300 wrong anyway (300 is almost never used in practice). So I
decided that instead of preserving the current wrong behaviour for 300,
it's better to just return it to the client.

Finally, that leaves 304 and 306.
* 306 is explicitly not used (says so in the spec).
* 304 is not a redirect, it's the answer to conditional GET/PUT
when the resource has not changed. So again it must go to the
caller, and not be treated as a redirect (it has no location
header so the current code fails)

The current code treats all other unknown 3xx codes as redirects and
looks for a location header. I think this is not a safe thing to do, we
cannot predict the behaviour of any newly allocated 3xx codes, so we
should just return them to the caller.

This simplifies the logic because it means we only match the ones we
explicitly handle, and return the others to the caller. Because we no
longer have the catch-all redirect case, then the 'redirect' function is
dead code.

Clear as mud?